Skip to content

refactor: Refactored the way of warnings#864

Merged
joein merged 16 commits intodevfrom
refactor-warnings
Jan 6, 2025
Merged

refactor: Refactored the way of warnings#864
joein merged 16 commits intodevfrom
refactor-warnings

Conversation

@hh-space-invader
Copy link
Copy Markdown
Contributor

@hh-space-invader hh-space-invader commented Dec 16, 2024

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you installed pre-commit with pip3 install pre-commit and set up hooks with pre-commit install?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@netlify
Copy link
Copy Markdown

netlify Bot commented Dec 16, 2024 •
edited
Loading

✅ Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit 8823786
🔍 Latest deploy log https://app.netlify.com/sites/poetic-froyo-8baba7/deploys/677bf461b00b4d0008a69862
😎 Deploy Preview https://deploy-preview-864--poetic-froyo-8baba7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@joein
Copy link
Copy Markdown
Member

joein commented Dec 16, 2024

  1. CI is failing
  2. Warnings should point to the users code which led to a warning, try applying such code to get the idea:
def create_payload_index(
        self,
        collection_name: str,
        field_name: str,
        field_schema: Optional[types.PayloadSchemaType] = None,
        field_type: Optional[types.PayloadSchemaType] = None,
        **kwargs: Any,
    ) -> types.UpdateResult:
        show_warning_once(
            message="Payload indexes have no effect in the local Qdrant. Please use server Qdrant if you need payload indexes.",
            category=UserWarning,
            idx="create-payload-index",
            stacklevel=5,
        )
        return self._default_update_result()

and running this code:

/.../qdrant_client/check_depr.py:59: UserWarning: Payload indexes have no effect in the local Qdrant. Please use server Qdrant if you need payload indexes.
  cl.create_payload_index('cn', 'payload')

Clicking on /.../qdrant_client/check_depr.py:59 path should lead to cl.create_payload_index call

@joein joein requested a review from I8dNLo December 18, 2024 12:26
joein
joein requested changes Dec 18, 2024
View reviewed changes
Comment thread qdrant_client/common/client_warnings.py Outdated
Comment thread qdrant_client/local/qdrant_local.py Outdated
Comment thread qdrant_client/qdrant_remote.py Outdated
Comment thread qdrant_client/qdrant_remote.py Outdated
Comment thread qdrant_client/qdrant_remote.py Outdated
Comment thread qdrant_client/qdrant_remote.py Outdated
Comment thread qdrant_client/qdrant_remote.py Outdated
Comment thread qdrant_client/uploader/rest_uploader.py Outdated
Comment thread qdrant_client/uploader/grpc_uploader.py Outdated
joein
joein reviewed Dec 31, 2024
View reviewed changes
Comment thread qdrant_client/qdrant_remote.py Outdated
@hh-space-invader hh-space-invader requested a review from joein January 3, 2025 08:49
joein
joein requested changes Jan 3, 2025
View reviewed changes
Comment thread qdrant_client/local/qdrant_local.py Outdated
Comment on lines +80 to +84
show_warning(
message=f"Collection appears to be None before closing. The existing collections are: "
f"{list(self.collections.keys())}",
category=UserWarning,
stacklevel=5,
Copy link
Copy Markdown
Member

@joein joein Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stacklevel should be 4 and it should also be show_warning_once, since we don't provide collection name here

the output with stacklevel=5:

sys:1: UserWarning: Collection appears to be None before closing. The existing collections are: ['test']

stacklevel=4:

/.../qdrant_client/check_warnings/local_close_collection.py:6: UserWarning: Collection appears to be None before closing. The existing collections are: ['test']
  cl.close()
/.../qdrant/qdrant_client/qdrant_client/qdrant_client.py:157: UserWarning: Collection appears to be None before closing. The existing collections are: ['test']

the script I used to reproduce:

from qdrant_client import QdrantClient, models

cl = QdrantClient(":memory:")
cl.create_collection('test', vectors_config=models.VectorParams(size=2, distance=models.Distance.COSINE))
cl._client.collections = {x: None for x in cl._client.collections}
cl.close()

Comment thread qdrant_client/local/qdrant_local.py Outdated
"with large datasets.",
category=UserWarning,
idx="large-local-collection",
stacklevel=6,
Copy link
Copy Markdown
Member

@joein joein Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be show_warning, since we are providing collection name here

Comment thread qdrant_client/qdrant_remote.py Outdated
warnings.warn("Auth token provider is used with an insecure connection.")
show_warning(
message="Auth token provider is used with an insecure connection.",
category=RuntimeWarning,
Copy link
Copy Markdown
Member

@joein joein Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is UserWarning

Comment thread qdrant_client/qdrant_remote.py Outdated
warnings.warn("Api key is used with an insecure connection.")
show_warning(
message="Api key is used with an insecure connection.",
category=RuntimeWarning,
Copy link
Copy Markdown
Member

@joein joein Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is UserWarning

@joein
Copy link
Copy Markdown
Member

joein commented Jan 3, 2025

Also

qdrant-client/qdrant_client/async_qdrant_remote.py

Line 156 in 9b89888

async def close(self, grpc_grace: Optional[float] = None, **kwargs: Any) -> None:

is still using logging.warning

Should be updated here

qdrant-client/tools/async_client_generator/transformers/remote/function_def_transformer.py

Line 38 in 9b89888

def override_close() -> ast.stmt:

@joein joein self-requested a review January 6, 2025 15:42
@joein joein merged commit 47ff758 into dev Jan 6, 2025
joein added a commit that referenced this pull request Jan 16, 2025
@hh-space-invader @joein
refactor: Refactored the way of warnings (#864)
908977e 
* refactor: Refactored the way of warnings

* remove unused imports

* fix: Fix stacklevel in warnings

* regenerated async

* nit

* Updated some warnings with show once

* fix: Fix stack levels

* Updated async

* Update qdrant_client/qdrant_remote.py

Co-authored-by: George <george.panchuk@qdrant.tech>

* Update async client

* Updated warnings

* Updated warnings

* Updated warnings

* fix: fix warning level

* fix: fix warning level in async

* fix: revert append payload condition

---------

Co-authored-by: George <george.panchuk@qdrant.tech>
@hh-space-invader hh-space-invader deleted the refactor-warnings branch April 8, 2025 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joein joein joein approved these changes

@I8dNLo I8dNLo Awaiting requested review from I8dNLo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hh-space-invader @joein @I8dNLo